Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable New Architecture in Bluesky on RN 0.76 #6755

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

WoLewicki
Copy link

PR enabling New Architecture in the app. Based on work from @haileyok in #6211 and a bit on #4853. Great work on those PRs so far 💪

There are still some issues like small input text on iOS, glitches with bottomsheet on both platforms and probably some more.

@WoLewicki
Copy link
Author

I'm happy to help investigate some of the issues if you'd like it @haileyok. We've already got much experience with all things New Arch related, like:

@WoLewicki WoLewicki changed the title @wolewicki/new arch 76 Enable New Architecture in Bluesky on RN 0.76 Nov 26, 2024
@mozzius mozzius marked this pull request as ready for review November 26, 2024 15:08
@mozzius mozzius marked this pull request as draft November 26, 2024 15:08
@gaearon
Copy link
Collaborator

gaearon commented Dec 12, 2024

Let's temporarily remove that package on this branch so that you're not blocked by that. I raised the problem to bitdrift maintainers.

@gaearon
Copy link
Collaborator

gaearon commented Dec 12, 2024

Pushed temporary reverts to this branch.

@WoLewicki
Copy link
Author

I have it working already on iOS but Android does not let me download the package for some reason and it doesn't look connected to the new arch. Working on a fix for bottomsheet too.

@gaearon
Copy link
Collaborator

gaearon commented Dec 12, 2024

Just got this branch running on Android. Nice job! I’m noticing that the home header is glitchy. It seems to flash a wrong state every now and then if you swipe up and down a few times.

IMG_2305.mov

@WoLewicki
Copy link
Author

WoLewicki commented Dec 13, 2024

@gaearon I made bottomSheet work on new arch, the issue was due to view flattening and not implementing needed methods in swift module.

I also added patch for @bitdrift/react-native too, but cannot do the same on Android due to some problems with downloading the sdk. Would be nice to have it tested pretty thoroughly 🚀

@WoLewicki
Copy link
Author

Ok I merged the newest main and bumped bitdrift package and now it works correctly on iOS 🚀 . Still, I seem to not have permission to download bitdrift android package so I cannot test there. What would be the next steps @gaearon ? Do you maybe have a QA team to test and find all the issues?
image

app.config.js Outdated Show resolved Hide resolved
@gaearon
Copy link
Collaborator

gaearon commented Dec 18, 2024

Have you had a chance to look at the cause of #6755 (comment)? I don't believe this was happening with Paper.

@WoLewicki
Copy link
Author

If I understand it correctly, the header animation is controlled here: https://github.com/bluesky-social/social-app/blob/a3031de19ba41717c8e83b818e294d44577fb434/src/view/com/util/MainScrollProvider.tsx yeah? And then the value is interpreted in transformY here:

. I'm logging the value and seems like headerHeight and when I scroll it down slowly, the headerModeValue is decreasing without any jumps in the value and at the same time, the header jumps a little sometimes, so it must be a problem somewhere deeper. I am no expert in reanimated unfortunately 😞

@gaearon
Copy link
Collaborator

gaearon commented Dec 19, 2024

Hm, we can't merge without getting to the root of it. It would be nice to reduce this to a minimal reproducing example (just a single file like App.native.tsx) and then report it to Reanimated, if possible.

@bartlomiejbloniarz
Copy link

@gaearon We looked into it with @WoLewicki more. The issue is that when no layout styles are updated, reanimated goes through a fast path, applying props synchronously on the UI thread. This fast path avoids our mechanisms that ensure the correct order of our updates. This results in the header jumping.

The fast path causes also other problems, and we want to get rid of it at some point - the problem is that would mean bad performance on lower end devices (so we need more time to figure out how to properly optimize this).

We tried to remove this fast path for your case, but for some reason the animation appears laggy (but in a different way - now the header doesn't jump back and forth, it just looks like it's skipping frames). It's weird because the UI thread is fairly unoccupied, so it seems that the animation for some reason is influenced by the heavy load that is done on the JS thread. We added an infinite animation there, and it seems that the performance is directly related to scrolling - we need to investigate that further.

@gaearon
Copy link
Collaborator

gaearon commented Dec 19, 2024

Makes sense. Thanks for the details.

@bartlomiejbloniarz
Copy link

@gaearon We came back to the office yesterday and I was finally able to dig deeper into the issue. As I said before the laggy animation is a result of the JS thread being heavily utilized (since we need to synchronize the updates applied on the JS thread with the animation). This is what it looks like:
Screenshot 2025-01-08 at 12 02 41
The complete root blocks can last up to 150ms, which is scary because they seem to be continuously invoked. These calls are usually a result of a rerender, but in this case something weirder is happening:

  • as you scroll the _onScroll method of VirtualizedList is called. It invokes _scheduleCellsToRenderUpdate which is basically a Batchinator wrapper for _updateCellsToRender
  • _updateCellsToRender calls setState here. The state manages which elements of the list should be rendered. The function that is passed to setState here often returns null - which means that there is no update - so no rerenders should happen.
  • but the complete root function is called here anyway

After some digging it seems that the <AppProfiler/> component used here is the reason this happens. Somehow the renderer decides that even if the state update of the list bails out with no updates, this component needs to be cloned and a new ShadowTree needs to be commited. It's possibly an issue with the react Profiler component. I also didn't see the any updates from the Profiler component in my console. Is it not supposed to run in debug builds?

This is the trace after I removed it from the app:
Screenshot 2025-01-08 at 12 04 23

The trace looks better now (and the animation seems to work better) - there is still some room for improvement here (but on the reanimated side). The JS thread now is mostly occupied with c++ state updates that are invoked to synchronize the scroll position with the ShadowTree. Those calls could probably be made faster by avoiding some unnecessary reanimated calculations.

@gaearon
Copy link
Collaborator

gaearon commented Jan 8, 2025

Somehow the renderer decides that even if the state update of the list bails out with no updates, this component needs to be cloned and a new ShadowTree needs to be commited.

Could be an RN bug! But not sure how to file it. Can you reproduce the same in a minimal app? I'm not sure what the reproducing instructions would be.

@gaearon
Copy link
Collaborator

gaearon commented Jan 8, 2025

We can definitely remove it to work around.

@gaearon gaearon mentioned this pull request Jan 8, 2025
@bartlomiejbloniarz
Copy link

bartlomiejbloniarz commented Jan 9, 2025

Could be an RN bug! But not sure how to file it. Can you reproduce the same in a minimal app? I'm not sure what the reproducing instructions would be.

Yes, I was able to reproduce it in an expo app with simply this code.
I checked and the completeRoot function is called even if the state wasn't updated only when the Profiler is used.

Code
import { Profiler } from "react";
import { StyleSheet, Text, View } from "react-native";
import Animated from "react-native-reanimated";

const data = new Array(200);

function onRender(id: string, phase: string, actualDuration: number) {}

export default function App() {
  return (
    <Profiler id="app" onRender={onRender}>
      <View style={styles.container}>
        <Animated.FlatList
          data={data}
          renderItem={(x) => {
            return (
              <View style={{ backgroundColor: "red", width: 200, height: 300 }}>
                <Text>{x.index}</Text>
              </View>
            );
          }}
        />
      </View>
    </Profiler>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: "center",
  },
  message: {
    textAlign: "center",
    paddingBottom: 10,
  },
  camera: {
    flex: 1,
  },
  buttonContainer: {
    flex: 1,
    flexDirection: "row",
    backgroundColor: "transparent",
    margin: 64,
  },
  button: {
    flex: 1,
    alignSelf: "flex-end",
    alignItems: "center",
  },
  text: {
    fontSize: 24,
    fontWeight: "bold",
    color: "white",
  },
});

@WoLewicki
Copy link
Author

Ok perfect! I will merge main here soon and check the performance with Profiler removed.

@WoLewicki
Copy link
Author

Ok I merged the newest main and Android header works better now. What are the next steps here then?

@gaearon
Copy link
Collaborator

gaearon commented Jan 10, 2025

I'm working on a few gesture-related fixes so we'll need to make sure those don't regress with the transition. Let me finish up that work first and then I'll give this PR another spin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants